Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add rustfmt raw identifer sorting doc #321

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

calebcartwright
Copy link
Member

@calebcartwright calebcartwright commented Sep 4, 2024

@joshtriplett
Copy link
Member

👍 This looks good.

I don't think this is captured in the style guide, and it should be.

@ehuss
Copy link
Contributor

ehuss commented Sep 4, 2024

BTW, new pages need to be added to src/SUMMARY.md.

I was wondering about the rustfmt changes and whether or not they all need to be on separate pages. I'm unclear about how many changes (from the list) will end up in the edition. If individual changes don't have significant text associated with them, I was wondering if putting them on the same page might make sense (especially if the Migration section is identical for each one).

However, if a single page ends up being really long, then that probably wouldn't be good.

Not something we need to decide right now, but would be good to think about.

@traviscross
Copy link
Contributor

traviscross commented Sep 4, 2024

Yes, I had similar thoughts in both directions, as you described, and ended up suggesting just doing one per page to @calebcartwright. That makes it a bit easier to follow the flow and template of the other pages, allows us to merge these individually without conflicts, etc. We can always merge them into one page later if we find it better editorially.

@calebcartwright
Copy link
Member Author

Agreed. I'll continue filing these as individual pages but I think most of them would be best merged into one (otherwise I could see a disproportionately high ratio of text in the edition guide doc being "run cargo fmt" repeated ad nauseam)

At this point I'm not entirely decided on which ones I think would be best combined on one page, but I'm leaning towards following the model we did for the Style Guide changelog where we had a single entry for "misc rustfmt fixes", which includes this raw ident sorting item.

If it doesn't add any harm/complexity for those of you driving the edition work, perhaps we could just leave these rustfmt PRs open here so that t-style/t-rustfmt can provide input on the text, and then I'll copy pasta things around (with relevant SUMMARY.md entries) and close some of the PRs once we figure out how to structure the full set?

@ehuss
Copy link
Contributor

ehuss commented Sep 6, 2024

Seems fine to me!

```

[Rust Style Guide]: https://doc.rust-lang.org/nightly/style-guide/index.html
[sorting]: https://doc.rust-lang.org/stable/style-guide/index.html?highlight=sort#sorting
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion to remove the highlighting.

Suggested change
[sorting]: https://doc.rust-lang.org/stable/style-guide/index.html?highlight=sort#sorting
[sorting]: https://doc.rust-lang.org/stable/style-guide/index.html#sorting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants